Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow _completion command to auto-complete it's arguments or their values #47

Conversation

aik099
Copy link
Contributor

@aik099 aik099 commented May 12, 2015

Right now the CompletionCommand has no arguments, but there might be added in #30. In that case without this PR the auto-complete for app _completion argument won't work.

In unit tests we're testing all parts separately, but we don't test by really invoking completion hook. That's why I don't know how do I test the changes I've made. All existing tests still pass.

Closes #46

@stecman
Copy link
Owner

stecman commented May 12, 2015

Nice spotting. This might be achievable with less code:

diff --git a/src/CompletionHandler.php b/src/CompletionHandler.php
index 0925df0..cce8fb4 100644
--- a/src/CompletionHandler.php
+++ b/src/CompletionHandler.php
@@ -103,6 +103,10 @@ class CompletionHandler

         try {
             $this->command = $this->application->find($cmdName);
+
+            // Create a new instance of the command to avoid any changes made by Command::run
+            $commandClass = get_class($this->command);
+            $this->command = new $commandClass();
         } catch (\InvalidArgumentException $e) {
             // Exception thrown, when multiple or none commands are found.
         }

I guess that doesn't cover cases where the constructor of a command has been changed though..

@aik099
Copy link
Contributor Author

aik099 commented May 13, 2015

That might work, but the application won't be available from the command and we need to do ->setApplication call too. And then it would look like code duplication. So using getNativeDefinition is recommended way. I'll make createDefinition method protected (it's private now because I copy-pased it from ListCommand) and people then can change it to alter command definition.

@aik099 aik099 force-pushed the 46-unable-to-auto-complete-for-completion-command branch from 4d44526 to d70320a Compare May 13, 2015 06:57
stecman added a commit that referenced this pull request May 13, 2015
…pletion-command

Allow `_completion` command to auto-complete it's arguments or their values
@stecman stecman merged commit 35659d2 into stecman:master May 13, 2015
@aik099 aik099 deleted the 46-unable-to-auto-complete-for-completion-command branch May 13, 2015 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to auto-complete for _completion command
2 participants